Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| writer("Write about AI agents") | ||
| ``` | ||
| """ | ||
| if not name: |
There was a problem hiding this comment.
Issue: Empty string check uses falsy evaluation which is inconsistent.
Suggestion: Consider using explicit is None checks for clarity. Currently if not name would treat an empty string "" the same as None, which may not be the intended behavior:
if name is None:
name = self.name
if description is None:
description = self.description or f"Use the {name} tool to invoke this agent as a tool"This makes the API contract clearer - None means "use default" while an explicit empty string could be considered user error.
src/strands/agent/agent_as_tool.py
Outdated
| Yields: | ||
| ToolStreamEvent for intermediate events, then ToolResultEvent with the final response. | ||
| """ | ||
| prompt = tool_use["input"].get("input", "") if isinstance(tool_use["input"], dict) else tool_use["input"] |
There was a problem hiding this comment.
Issue: The input extraction logic handles two cases but could silently fail with unexpected input formats.
Suggestion: Consider adding explicit type handling or validation:
tool_input = tool_use["input"]
if isinstance(tool_input, dict):
prompt = tool_input.get("input", "")
elif isinstance(tool_input, str):
prompt = tool_input
else:
logger.warning("tool_name=<%s> | unexpected input type: %s", self._tool_name, type(tool_input))
prompt = str(tool_input)|
Issue: The original issue #1002 mentions a Question: Is this parameter intentionally deferred to a follow-up PR, or should it be included in this initial implementation? The current implementation appears to preserve context by default (since it uses the same agent instance), but there's no way for users to reset context between calls. This could be a breaking change if added later, so it would be good to clarify the intended behavior. |
Code Review SummaryAssessment: Request Changes This PR introduces a well-designed Review Categories
The core implementation looks solid - just needs the documentation PR linked to unblock merge. |
|
/strands review |
Code Review Summary - UpdatedAssessment: Approve ✅ Thank you for addressing the feedback from the previous review! All blocking issues have been resolved: Changes Addressed
Minor Notes (Non-blocking)
The implementation is well-designed, follows SDK patterns, and includes proper error handling and logging. Ready to merge once the awsarron discussion is resolved and any CI checks pass. |
src/strands/agent/agent_as_tool.py
Outdated
| "type": "string", | ||
| "description": "The input to send to the agent tool.", | ||
| }, | ||
| "preserve_context": { |
There was a problem hiding this comment.
This should be a class level field. I think most consumers want to decide at creation time whether it's a persistent tool (preserve_context=True) or new agent call on every invocation (preserve_context=False).
I also wonder if there's a way we can determine the best default - if not, maybe make it required
There was a problem hiding this comment.
Swapped to class level field with default True. I think we should take the least destructive default
There was a problem hiding this comment.
Our docs implementation seems to correspond to preserve_context=False: https://strandsagents.com/docs/user-guide/concepts/multi-agent/agents-as-tools/
I just realized, preserve_context=True might need some sort of integration with the parent agent's AgentState, as otherwise if an agent is persisted & rehydrated the context of the sub-agent isn't restored. Maybe that's alright, as it's a tool concern, bug given that it's a tool that we're vending, we should at least explore if an agent-as-tool should integrate with the parent's AgentState
src/strands/agent/agent_as_tool.py
Outdated
| self._tool_name, | ||
| tool_use_id, | ||
| ) | ||
| messages.clear() |
There was a problem hiding this comment.
I don't think we want to clear the agent in this case; we want to restore it to the original state that it was at. The graph node does something similar I think.
In the future, we can use snapshots to simplify this.
AgentState is another one that will need to be restored
There was a problem hiding this comment.
took the pattern from graph.py
| def test_init_sets_name(mock_agent): | ||
| tool = AgentAsTool(mock_agent, name="my_tool", description="desc") | ||
| assert tool.tool_name == "my_tool" | ||
|
|
||
|
|
||
| def test_init_sets_description(mock_agent): | ||
| tool = AgentAsTool(mock_agent, name="my_tool", description="custom desc") | ||
| assert tool._description == "custom desc" | ||
|
|
||
|
|
||
| def test_init_stores_agent_reference(mock_agent, tool): | ||
| assert tool.agent is mock_agent |
| def test_tool_name(tool): | ||
| assert tool.tool_name == "test_agent" | ||
|
|
||
|
|
||
| def test_tool_type(tool): | ||
| assert tool.tool_type == "agent" | ||
|
|
||
|
|
||
| def test_tool_spec_name(tool): | ||
| assert tool.tool_spec["name"] == "test_agent" | ||
|
|
||
|
|
||
| def test_tool_spec_description(tool): | ||
| assert tool.tool_spec["description"] == "A test agent" | ||
|
|
||
|
|
||
| def test_tool_spec_input_schema(tool): | ||
| schema = tool.tool_spec["inputSchema"]["json"] | ||
| assert schema["type"] == "object" | ||
| assert "input" in schema["properties"] | ||
| assert schema["properties"]["input"]["type"] == "string" | ||
| assert "preserve_context" in schema["properties"] | ||
| assert schema["properties"]["preserve_context"]["type"] == "boolean" | ||
| assert schema["required"] == ["input"] |
There was a problem hiding this comment.
Do an assert on the entire spec at once
| props = tool.get_display_properties() | ||
| assert props["Agent"] == "test_agent" | ||
| assert props["Type"] == "agent" |
There was a problem hiding this comment.
Same here - assert on the entire object
src/strands/agent/agent_as_tool.py
Outdated
| if "result" in event: | ||
| result = event["result"] | ||
| else: | ||
| yield event |
There was a problem hiding this comment.
We will auto wrap this in a ToolStreamResult, but I think we actually want to do additional wrapping here - Maybe an AgentAsToolStreamEvent?
If we don't, the caller cannot tell the difference between "This is ToolStreamEvent that came from an tool wrapping an agent using a tool" vs "This is a ToolStreamEvent from a tool"
AgentAsToolStreamEvent would probably reference the AgentAsTool class
There was a problem hiding this comment.
Added AgentAsToolStreamEvent class that is a subclass of ToolStreamEvent to differentiate
src/strands/agent/agent.py
Outdated
| if not name: | ||
| name = self.name | ||
| if not description: | ||
| description = self.description or f"Use the {name} tool to invoke this agent as a tool" |
There was a problem hiding this comment.
We'll need a better description I think
invoke this agent as a tool
Doesn't really make sense to an agent. It's going to see a tool that says:
name: "researcher"
description: "Use the researcher tool to invoke this agent as a tool"
where this doesn't make sense.
There was a problem hiding this comment.
swapped to Use the {name} agent as a tool by providing a natural language input
src/strands/agent/agent_as_tool.py
Outdated
|
|
||
| tool_use_id = tool_use["toolUseId"] | ||
|
|
||
| if not preserve_context: |
There was a problem hiding this comment.
We should throw if preserve_context=False and _agent != Agent; that would indicate A2A agent which for preserve_context=False doesn't make sense
There was a problem hiding this comment.
put this check in the constructor
There was a problem hiding this comment.
Let's rename this as _agent_as_tool.py and just export at the top level
…e; yield AgentAsToolStreamEvents; small fixes
|
/strands review |
|
/strands review if there are any issues with the last logic changes (+ general review) |
Code Review Summary - Round 4 (Logic Changes Focus)Assessment: Approve ✅ Reviewed the latest logic changes including concurrency handling, interrupt propagation, and default value changes. The implementation is solid. Key Logic Changes Reviewed
Concurrency HandlingThe lock implementation is correct:
Interrupt Flow
No issues found with the logic changes. The implementation handles edge cases correctly and includes comprehensive test coverage (672 lines covering concurrency, interrupts, state management). |
Decision Log:
|
| Type | as_tool() available? |
Default works? | Must set preserve_context=True? |
|---|---|---|---|
Agent |
✅ Yes (method on Agent) |
✅ Yes | No |
AgentBase subclass (A2A, custom) |
❌ No (as_tool not on AgentBase) |
N/A | Must construct AgentAsTool directly |
AgentBase + AgentAsTool(...) directly |
N/A | ❌ Raises TypeError |
✅ Yes, mandatory |
Is This Actually a Problem?
Not really. The as_tool() convenience method only exists on Agent, so the default preserve_context=False always has a valid target. A2A agents can't accidentally hit this because they'd have to go through AgentAsTool(...) directly, which raises a clear TypeError.
The error message is explicit:
TypeError: preserve_context=False requires an Agent instance, got MyCustomAgent
Why This Design?
AgentBase doesn't guarantee messages or state attributes exist. You can't snapshot/restore what doesn't exist. So the type check is correct — it's a capability check, not an arbitrary restriction.
📋 Test Results (13/13 passing)
TestPreserveContextWithSessionManager::test_reset_does_not_trigger_message_added_hooks PASSED
TestPreserveContextWithSessionManager::test_session_manager_initialize_runs_on_sub_agent_construction PASSED
TestPreserveContextWithSessionManager::test_session_message_index_diverges_after_reset PASSED
TestPreserveContextWithSessionManager::test_sync_agent_after_reset_persists_wrong_state PASSED
TestRaceConditions::test_reset_then_stream_is_not_atomic_with_session_hooks PASSED
TestRaceConditions::test_session_latest_message_not_reset PASSED
TestPreserveContextDefault::test_default_raises_for_agent_base PASSED
TestPreserveContextDefault::test_preserve_context_true_works_for_agent_base PASSED
TestPreserveContextDefault::test_default_means_a2a_agents_must_use_preserve_context_true PASSED
TestFundamentalIssue::test_document_the_divergence_scenario PASSED
TestFundamentalIssue::test_verify_session_hooks_registered_on_sub_agent PASSED
TestConversationManagerInteraction::test_conversation_manager_state_not_reset PASSED
TestConversationManagerInteraction::test_apply_management_runs_after_each_invocation PASSED
Tests verify code structure and behavior via source inspection + runtime type checking. No mocked model calls needed — these test the interaction patterns, not the end-to-end flow.
💡 Recommendations
For This PR
-
Document the session manager interaction in the docstring — Add a note to
preserve_contextparameter docs that it does NOT coordinate with session managers. Developers who set both will get in-memory amnesiac but persistent accumulation. -
Consider a warning — When
preserve_context=Falseand the wrapped agent has a_session_manager, log a warning:if not preserve_context and hasattr(agent, '_session_manager') and agent._session_manager: logger.warning("preserve_context=False with a session manager may cause state divergence")
For Future Work
-
Unified state management — Both
GraphNode.reset_executor_state()andAgentAsTool._reset_agent_state()have the same pattern and same gap. This should move to a first-class method onAgent(e.g.,agent.reset_to_snapshot(snapshot)) that properly coordinates with session managers and conversation managers. -
Session-aware reset — The reset method should either:
- Clear session messages and reset the
_latest_agent_messagetracker, OR - Detach the session manager during sub-agent tool invocations (the sub-agent's session is the parent's concern, not its own)
- Clear session messages and reset the
🤖 AI agent response. Strands Agents. Feedback welcome!
|
^ following up on the findings couple of things:
|
📚 Research: Agent-as-Tool Context Management — How Other SDKs Handle ItCross-Framework AnalysisI went through the actual source code of Google ADK, OpenAI Agents SDK, CrewAI, and LangGraph to understand how they handle context/message history in agent-as-tool patterns. Summary Table
Key Finding: Strands Is Unique in HOW It Achieves IsolationEvery other SDK achieves isolation by creating fresh infrastructure around the agent (new runner, new session, new task). Strands achieves it by mutating the agent directly ( This is why What Other SDKs Do
The TensionAll SDKs agree: agent-as-tool should default to fresh/isolated context. But Strands has a unique challenge: when developers construct an Agent with specific components (session managers, conversation managers) and pass it as a tool, RecommendationThree options discussed in the detailed analysis:
Long-term: the isolation mechanism should follow the ADK/OpenAI pattern (fresh execution context around the agent) rather than mutating the agent directly. AI agent response. Strands Agents. Feedback welcome! |
…n the constructor
src/strands/agent/__init__.py
Outdated
| "AgentBase", | ||
| "AgentResult", | ||
| "AgentAsTool", | ||
| "_AgentAsTool", |
src/strands/agent/agent.py
Outdated
|
|
||
| Returns: | ||
| An AgentAsTool wrapping this agent. | ||
| An _AgentAsTool wrapping this agent. |
There was a problem hiding this comment.
nit: For customers this does not mean anything. Instead, you can say "a tool wrapping the agent". At the end of the day _AgentAsToolis an AgentTool
There was a problem hiding this comment.
Actually I am even thinking if this should return an agent tool. Because _AgentAsTool is not public, and this is a public API. cc @zastrowm wdyt?
The biggest downside of that would be, we cannot go back to AgentAsTool if/when we make it public, that'd be technically backwards incompatible change.
There was a problem hiding this comment.
Actually I am even thinking if this should return an agent tool.
Agree; this should return AgentTool
The biggest downside of that would be, we cannot go back to AgentAsTool
I think we can - because it'd be widening an output type and not an input - it's still assignable to AgentTool but it's more specific so all type checks should still work. Only small concern would be that if you overrode Agent.to_tool, but I don't think that's a big enough problem to block on.
There was a problem hiding this comment.
made the copy update in the docstring
There was a problem hiding this comment.
If its backwards incompatible then we should leave it as _AgentAsTool IMO
| @@ -327,25 +327,25 @@ def tool_use_id(self) -> str: | |||
| class AgentAsToolStreamEvent(ToolStreamEvent): | |||
There was a problem hiding this comment.
do we need a special stream event?
There was a problem hiding this comment.
Yes it is used to differentiate in what context the agent is being used for the customer: either as an agent directly or as a tool. Thread from before: #1932 (comment)
src/strands/__init__.py
Outdated
| __all__ = [ | ||
| "Agent", | ||
| "AgentAsTool", | ||
| "_AgentAsTool", |
There was a problem hiding this comment.
again, we are exporting private class. we should not do it
|
/strands review |
src/strands/__init__.py
Outdated
|
|
||
| __all__ = [ | ||
| "Agent", | ||
| "_AgentAsTool", |
There was a problem hiding this comment.
Issue: Private class exported in __all__.
The underscore prefix (_AgentAsTool) indicates this is a private/internal class, but exporting it in __all__ makes it part of the public API. This is contradictory.
Suggestion: Either:
- Remove
_AgentAsToolfrom__all__(keep it truly private), OR - Rename to
AgentAsTool(make it public)
Per @mkmeral's feedback, if this is intended to be private, it shouldn't be exported.
src/strands/agent/agent.py
Outdated
| interactions with the agent. Defaults to False. | ||
|
|
||
| Returns: | ||
| An _AgentAsTool wrapping this agent. |
There was a problem hiding this comment.
Issue: Return type references private class name.
"An _AgentAsTool wrapping this agent" is meaningless to customers since _AgentAsTool is a private class they shouldn't interact with directly.
Suggestion: Change to:
Returns:
A tool wrapping this agent that can be passed to another agent's tools list.
Or since _AgentAsTool extends AgentTool:
Returns:
An AgentTool wrapping this agent.
src/strands/agent/__init__.py
Outdated
| "Agent", | ||
| "AgentBase", | ||
| "AgentResult", | ||
| "_AgentAsTool", |
There was a problem hiding this comment.
Issue: Same as above - private class exported in __all__.
Remove _AgentAsTool from __all__ to keep it truly private.
Code Review Summary - Round 5Assessment: Request Changes The session manager divergence bug has been addressed with a Blocking Issues
What's Been Addressed
Design Decision Needed@mkmeral raised a valid point: should
The current state (private name + public export) is contradictory. I'd suggest option 1 (truly private) since customers have Once the export inconsistency is resolved, this is ready to merge. |
|
/strands review |
Code Review Summary - Round 6Assessment: Approve ✅ All blocking issues from previous reviews have been resolved. Issues Resolved
Final Implementation SummaryAPI Design:
Key Features:
Test Coverage: 97.20% with 676 lines of comprehensive tests The implementation is well-designed, follows SDK patterns, and addresses the original issue #1002 completely. Ready to merge! 🎉 |
Description
Will close #1002.
This PR adds the
AgentAsToolclass as well as a convenience method on theAgentclass to return an instance of it.The
AgentAsToolclass just callsstream_asyncon the underlying agent and formats the response as aToolResultEvent.The added integration test confirms that an AgentAsTool was invoked and that agent-as-tool used a tool of its own.
Related Issues
#1002
Documentation PR
strands-agents/docs#686
Type of Change
New feature
Testing
Added unit tests
Added a minimal integ test
[ x] I ran
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.